Repository navigation
fix(Select): guard empty results and retain modal popup scope - #752
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The Select portal still mispositions popups in transformed dialogs, allows focus to escape the modal when no options match, and excludes alertdialog containers from its scope lookup.
3 open issues
What changed in this PR
This PR addresses shared Select issues reported in mieweb/eSheet#256: keyboard handling after empty searches and popup containment within dialogs.
Changes:
- Guards empty-result navigation and selection against missing options.
- Targets the containing dialog for popup portals, with a body fallback.
- Adds keyboard and dialog-containment regressions and updates Storybook documentation.
| File | Description |
|---|---|
src/components/Select/Select.tsx |
Adds empty-result guards and dialog-scoped portal targeting. |
src/components/Select/Select.test.tsx |
Adds no-match keyboard and modal popup containment tests. |
src/components/Select/Select.stories.tsx |
Documents the revised portal behavior. |
Review effort: Balanced
Deploying ui with
|
| Latest commit: |
18e7e5f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://2eda5c02.ui-6d0.pages.dev |
| Branch Preview URL: | https://fix-select-empty-modal-scope.ui-6d0.pages.dev |
There was a problem hiding this comment.
🟡 Changes recommended
The popover’s default foreground overrides the dark theme’s text color, making unselected option labels black against a dark background; the panel needs an explicit foreground color and verification without Storybook’s dark color-scheme override.
1 open issue
3 resolved since last review
Review effort: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The new visual test’s token overrides lose to existing !important declarations, causing its color assertion to fail before the geometry regression check, and accessibility descriptions still contradict the implementation.
2 open issues
1 resolved since last review
Review effort: Balanced
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Native-popover modal scope and active-option announcements still require human assistive-technology validation, since DOM relationships and focus assertions cannot establish screen-reader behavior.
No open issues 🎉
2 resolved since last review
Review effort: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Native top-layer popup and active-descendant changes require human validation of screen-reader announcements and downstream integration beyond the supplied DOM and geometry assertions.
No open issues 🎉
Review effort: Balanced



Summary
Addresses the shared Select defects tracked in #753 and reported in mieweb/eSheet#256:
Validation
pnpm typecheckand fullpnpm lintpassed.Full downstream eSheet compose/upload integration and assistive-technology testing were not repeated during this follow-up. No npm release has been published by this update.